Conversation
WalkthroughModel-scoped API-key health and selection were added by introducing a Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ChatHandler as Chat Handler
participant KeySelector as Provider<br/>Key Selector
participant HealthTracker as Health<br/>Tracker
participant Provider as Upstream<br/>Provider
Client->>ChatHandler: Request (baseModelName)
ChatHandler->>KeySelector: findProviderKey(org, provider,<br/>selectionScope: baseModelName)
KeySelector->>HealthTracker: isTrackedKeyHealthy(keyId,<br/>selectionScope: baseModelName)
HealthTracker-->>KeySelector: healthy? (scoped)
KeySelector-->>ChatHandler: primaryKey
ChatHandler->>Provider: Send request with primaryKey
Provider-->>ChatHandler: Error (401/403/400 with invalid-key)
ChatHandler->>ChatHandler: shouldRetryAlternateKey(errorType,<br/>statusCode, errorText)?
alt Retry Eligible
ChatHandler->>KeySelector: findProviderKey(...,<br/>selectionScope: baseModelName,<br/>excludedKeyIds: [primaryKey])
KeySelector->>HealthTracker: isTrackedKeyHealthy(alternateKeyId,<br/>selectionScope: baseModelName)
HealthTracker-->>KeySelector: healthy? (scoped)
KeySelector-->>ChatHandler: alternateKey
ChatHandler->>HealthTracker: reportTrackedKeyError(primaryKeyId,<br/>statusCode, errorText,<br/>selectionScope: baseModelName)
ChatHandler->>Provider: Retry with alternateKey
Provider-->>ChatHandler: Success
ChatHandler->>HealthTracker: reportTrackedKeySuccess(alternateKeyId,<br/>selectionScope: baseModelName)
else Not Eligible
ChatHandler->>HealthTracker: reportTrackedKeyError(primaryKeyId,<br/>statusCode, errorText,<br/>selectionScope: baseModelName)
ChatHandler-->>Client: Return error
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~90 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
steebchen
changed the title
steebchen
force-pushed
the
fix/retry-invalid-api-key-payloads
branch
from
99f947c to
b49cce3
Compare
There was a problem hiding this comment.
Pull request overview
This PR refines retry/health behavior across the gateway and worker so that (1) invalid-credential failures are detected even when upstream responds with 400, (2) API key health is tracked per-model (not globally per key), and (3) provider routing health stats aren’t degraded by same-provider retries that ultimately recover.
Changes:
- Add invalid-provider-credential text detection and use it in finish-reason classification, retry decisions, and key blacklisting.
- Scope key health tracking/selection by model via a
selectionScopeplumbed through env-key and DB-key selection paths. - Exclude same-provider recovered retry attempts from worker-generated routing health statistics, with supporting tests.
Reviewed changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| apps/worker/src/services/stats-calculator.ts | Filters out recovered same-provider retry attempts from minute-level stats aggregation. |
| apps/worker/src/services/stats-calculator.spec.ts | Adds coverage ensuring recovered same-provider retries don’t degrade mapping/model health stats. |
| apps/gateway/src/test-utils/mock-openai-server.ts | Adds a mock scenario for invalid-key payloads returned as 400 on first attempt. |
| apps/gateway/src/lib/round-robin-env.ts | Threads selectionScope into env key selection so health is isolated by scope. |
| apps/gateway/src/lib/round-robin-env.spec.ts | Tests env-key health isolation per model scope. |
| apps/gateway/src/lib/provider-auth-errors.ts | New helper for detecting invalid credential error payload patterns. |
| apps/gateway/src/lib/cached-queries.ts | Threads selectionScope into DB provider-key selection and health checks. |
| apps/gateway/src/lib/cached-queries.spec.ts | Tests tracked-key health isolation per model scope. |
| apps/gateway/src/lib/api-key-health.ts | Adds model-scoped health keys and uses invalid-credential text detection for permanent blacklisting. |
| apps/gateway/src/lib/api-key-health.spec.ts | Adds a test for permanently blacklisting invalid-key text on an otherwise-ignored 4xx. |
| apps/gateway/src/graceful-shutdown.spec.ts | Stabilizes tests by waiting for the server to be listening before making requests. |
| apps/gateway/src/fallback.spec.ts | Adds fallback tests for same-provider key rotation on auth failures and invalid-key payloads; resets key health between tests. |
| apps/gateway/src/chat/tools/retry-with-fallback.ts | Introduces shouldRetryAlternateKey to allow same-provider key rotation for auth/invalid-key payloads. |
| apps/gateway/src/chat/tools/retry-with-fallback.spec.ts | Adds unit tests for shouldRetryAlternateKey. |
| apps/gateway/src/chat/tools/resolve-provider-context.ts | Passes base model as selectionScope into env/key selection. |
| apps/gateway/src/chat/tools/get-provider-env.ts | Accepts selectionScope and forwards it into round-robin env key selection. |
| apps/gateway/src/chat/tools/get-provider-env.spec.ts | Tests that selectionScope affects env key health/selection. |
| apps/gateway/src/chat/tools/get-finish-reason-from-error.ts | Treats invalid-key payload text as credential failure (gateway_error) even for 400. |
| apps/gateway/src/chat/tools/get-finish-reason-from-error.spec.ts | Adds coverage for invalid-key payloads on 400. |
| apps/gateway/src/chat/chat.ts | Plumbs model scope into key health reporting and uses shouldRetryAlternateKey for same-provider retries. |
| apps/gateway/src/api.spec.ts | Updates tracked-key health assertions to use scoped health keys. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| */ | ||
| export async function findCustomProviderKey( | ||
| organizationId: string, | ||
| customProviderName: string, | ||
| _selectionKey?: string, | ||
| selectionScope?: string, | ||
| excludedKeyIds?: ReadonlySet<string>, | ||
| ): Promise<ProviderKey | undefined> { | ||
| const results = await db | ||
| .select() | ||
| .from(providerKeyTable) | ||
| .where( | ||
| and( | ||
| eq(providerKeyTable.status, "active"), | ||
| eq(providerKeyTable.organizationId, organizationId), | ||
| eq(providerKeyTable.provider, "custom"), | ||
| eq(providerKeyTable.name, customProviderName), | ||
| ), | ||
| ) | ||
| .orderBy(asc(providerKeyTable.createdAt), asc(providerKeyTable.id)); | ||
| return selectProviderKeyWithFailover(results, excludedKeyIds); | ||
| return selectProviderKeyWithFailover(results, selectionScope, excludedKeyIds); | ||
| } | ||
|
|
||
| /** | ||
| * Find a provider key by organization and provider (cacheable) | ||
| */ | ||
| export async function findProviderKey( | ||
| organizationId: string, | ||
| provider: string, | ||
| _selectionKey?: string, | ||
| selectionScope?: string, | ||
| excludedKeyIds?: ReadonlySet<string>, | ||
| ): Promise<ProviderKey | undefined> { | ||
| const results = await db | ||
| .select() | ||
| .from(providerKeyTable) | ||
| .where( | ||
| and( | ||
| eq(providerKeyTable.status, "active"), | ||
| eq(providerKeyTable.organizationId, organizationId), | ||
| eq(providerKeyTable.provider, provider), | ||
| ), | ||
| ) | ||
| .orderBy(asc(providerKeyTable.createdAt), asc(providerKeyTable.id)); | ||
| return selectProviderKeyWithFailover(results, excludedKeyIds); | ||
| return selectProviderKeyWithFailover(results, selectionScope, excludedKeyIds); |
There was a problem hiding this comment.
findProviderKey / findCustomProviderKey now treat the 3rd argument as selectionScope (used to scope in-memory key health buckets). There are still call sites in the repo passing requestId/selectionKey as the 3rd argument (e.g. apps/gateway/src/moderations/moderations.ts and apps/gateway/src/videos/videos.ts), which will accidentally scope health per-request and can cause unbounded keyHealthMap growth + inconsistent failover behavior. Please update those call sites to pass a stable scope (e.g. base model name) or undefined, and if request-based cache-busting is still needed, reintroduce it as a separate parameter rather than overloading selectionScope.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b45c134a5d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| metrics: getTrackedKeyMetrics(item.id, selectionScope), | ||
| })) | ||
| .filter(({ item }) => isTrackedKeyHealthy(item.id)); | ||
| .filter(({ item }) => isTrackedKeyHealthy(item.id, selectionScope)); |
There was a problem hiding this comment.
This change makes selectionScope participate in key-health lookups (getTrackedKeyMetrics / isTrackedKeyHealthy), but the third argument on findProviderKey/findCustomProviderKey was previously an ignored _selectionKey, and some callers still pass per-request IDs (for example apps/gateway/src/moderations/moderations.ts around lines 380-401 and apps/gateway/src/videos/videos.ts around line 2164). Because those values are unique per request while health reporting there remains unscoped, key selection effectively sees a fresh healthy bucket every time and can keep choosing bad keys instead of failing over.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/gateway/src/chat/chat.ts (2)
2694-2724:⚠️ Potential issue | 🟠 MajorKeep the tracked key identity in sync when a regional env token replaces the selected credential.
Lines 2719-2724, 2768-2773, 2794-2799, and 2845-2850 can swap
usedTokento a region-specific env value, butproviderKey,envVarName, andconfigIndexstill describe the original DB/base key. The laterrememberFailedKey/reportKey*calls will quarantine or heal the wrong credential, and same-provider retry can keep reusing the same bad regional token while rotating unrelated keys.Also applies to: 2761-2774, 2778-2800, 2838-2851
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/gateway/src/chat/chat.ts` around lines 2694 - 2724, The region-specific env token replacement swaps usedToken but leaves providerKey/envVarName/configIndex pointing at the original DB key, causing wrong keys to be reported or quarantined; update the logic in the blocks that call getRegionSpecificEnvValue (around where usedRegion is set after resolveRegionFromProviderKey) to also update the tracking identity: when regionToken replaces usedToken, set providerKey.token (or create a new providerKey-like object) and update envVarName and configIndex to reflect the env-derived credential so subsequent calls to rememberFailedKey and reportKey* operate on the actual credential in use; ensure the same change is applied to all similar blocks (the ones you noted at 2719-2724, 2768-2773, 2794-2799, 2845-2850 and the other mirrored ranges).
4846-4860:⚠️ Potential issue | 🟠 MajorDo not let recovered same-provider retries mark the provider as failed.
These branches now record failed attempts for alternate-key retries, but the later
failedMap/providerScoresenrichment still keys failures only by provider at Lines 5335-5351 and 8502-8516. If key A fails and key B succeeds on the same provider, the provider still ends up markedfailed, so derived provider uptime is still penalized.Possible follow-up
-const failedMap = new Map( - routingAttempts - .filter((a) => !a.succeeded) - .map((f) => [f.provider, f]), -); +const recoveredProviders = new Set( + routingAttempts.filter((a) => a.succeeded).map((a) => a.provider), +); +const failedMap = new Map( + routingAttempts + .filter((a) => !a.succeeded && !recoveredProviders.has(a.provider)) + .map((f) => [f.provider, f]), +);Also applies to: 5123-5137, 8231-8241
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/gateway/src/chat/chat.ts` around lines 4846 - 4860, The current logic calls rememberFailedKey during same-provider alternate-key retries which causes the provider to be treated as failed even if another key on the same provider later succeeds; update the flow so that when shouldRetryAlternateKey(...) triggers and you call tryResolveAlternateKeyForCurrentProvider(true) you do NOT mark the provider as failed up-front — instead only record the specific key failure (or pass a flag to rememberFailedKey to avoid marking provider-level failure), and only add entries to failedMap/providerScores (the provider-level failure enrichment used later) when all keys for that provider have actually failed or when there is no successful alternate key (i.e., check the result of tryResolveAlternateKeyForCurrentProvider and only escalate to provider-level failure if it returns falsy). Ensure changes touch the branches around shouldRetryAlternateKey, rememberFailedKey, tryResolveAlternateKeyForCurrentProvider and the later failedMap/providerScores enrichment so provider uptime isn’t penalized when an alternate key recovers the request.
🧹 Nitpick comments (1)
apps/gateway/src/lib/api-key-health.ts (1)
47-68: Consider monitoring memory growth with model-scoped health tracking.With model-scoped health keys,
keyHealthMapentries grow as O(keys × models) rather than O(keys). WhileMAX_HISTORY_SIZEandMETRICS_WINDOW_MSbound per-entry memory, there's no mechanism to prune stale entries for models no longer in use.For most deployments this is acceptable, but consider:
- Monitoring
getTrackedKeyCount()in production- Adding TTL-based eviction for entries with no recent activity if memory becomes a concern
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/gateway/src/lib/api-key-health.ts` around lines 47 - 68, The keyHealthMap can grow unbounded when using model-scoped keys; add a TTL-based eviction to prune entries with no recent activity: augment KeyHealth entries with a lastActivity timestamp updated in recordError/recordSuccess, expose getTrackedKeyCount() for monitoring, and implement a periodic cleanup task (interval based on METRICS_WINDOW_MS or a new CLEANUP_INTERVAL_MS) that removes map entries whose lastActivity is older than a configurable TTL (e.g., METRICS_WINDOW_MS or a new KEY_TTL_MS) while preserving MAX_HISTORY_SIZE semantics; reference keyHealthMap, KeyHealth, recordError/recordSuccess, getTrackedKeyCount, MAX_HISTORY_SIZE and METRICS_WINDOW_MS to locate where to update and where to add the cleanup timer.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/gateway/src/chat/chat.ts`:
- Around line 2694-2704: Calls to findCustomProviderKey and findProviderKey are
missing the new selectionScope argument in several places; update every
invocation (including the earlier call near the top and the ones that correspond
to the referenced blocks) to pass the current selectionScope value so the key
lookups are region/selection-scoped. Specifically, add selectionScope as the
third parameter when calling findCustomProviderKey(project.organizationId,
customProviderName, selectionScope) and when calling
findProviderKey(project.organizationId, usedProvider, selectionScope) (and
similarly for the other direct-provider calls noted), ensuring you use the same
selectionScope variable used elsewhere in this file so custom-provider
validation and region locking are correctly scoped.
In `@apps/gateway/src/lib/provider-auth-errors.ts`:
- Around line 1-5: The current INVALID_PROVIDER_CREDENTIAL_PATTERNS only matches
human-readable message text and misses structured error codes like
"invalid_api_key"; update the pattern array
(INVALID_PROVIDER_CREDENTIAL_PATTERNS) to include a case-insensitive regex that
matches the error code token (e.g. /\binvalid_api_key\b/i or a JSON-ish code
match) so payloads carrying code: "invalid_api_key" trigger the same
auth-failure handling, and apply the same change to the other identical pattern
array found later in the file (the second occurrence at lines 12-14).
In `@apps/gateway/src/test-utils/mock-openai-server.ts`:
- Around line 770-785: The invalid-key trigger handler (checking userMessage for
"TRIGGER_FAIL_ONCE_INVALID_KEY") is colliding with the more general
"TRIGGER_FAIL_ONCE" handler and causing failOnceCounter to be incremented twice;
make the triggers mutually exclusive by changing the control flow so only one
handler runs: either (A) convert the generic "TRIGGER_FAIL_ONCE" handler to an
else if so it won’t run when "TRIGGER_FAIL_ONCE_INVALID_KEY" matched, or (B)
make the invalid-key check more specific (e.g., exact match or
word-boundary/unique token) so
userMessage.includes("TRIGGER_FAIL_ONCE_INVALID_KEY") cannot also match the
generic "TRIGGER_FAIL_ONCE"; adjust code around failOnceCounter, userMessage,
TRIGGER_FAIL_ONCE_INVALID_KEY and TRIGGER_FAIL_ONCE accordingly.
---
Outside diff comments:
In `@apps/gateway/src/chat/chat.ts`:
- Around line 2694-2724: The region-specific env token replacement swaps
usedToken but leaves providerKey/envVarName/configIndex pointing at the original
DB key, causing wrong keys to be reported or quarantined; update the logic in
the blocks that call getRegionSpecificEnvValue (around where usedRegion is set
after resolveRegionFromProviderKey) to also update the tracking identity: when
regionToken replaces usedToken, set providerKey.token (or create a new
providerKey-like object) and update envVarName and configIndex to reflect the
env-derived credential so subsequent calls to rememberFailedKey and reportKey*
operate on the actual credential in use; ensure the same change is applied to
all similar blocks (the ones you noted at 2719-2724, 2768-2773, 2794-2799,
2845-2850 and the other mirrored ranges).
- Around line 4846-4860: The current logic calls rememberFailedKey during
same-provider alternate-key retries which causes the provider to be treated as
failed even if another key on the same provider later succeeds; update the flow
so that when shouldRetryAlternateKey(...) triggers and you call
tryResolveAlternateKeyForCurrentProvider(true) you do NOT mark the provider as
failed up-front — instead only record the specific key failure (or pass a flag
to rememberFailedKey to avoid marking provider-level failure), and only add
entries to failedMap/providerScores (the provider-level failure enrichment used
later) when all keys for that provider have actually failed or when there is no
successful alternate key (i.e., check the result of
tryResolveAlternateKeyForCurrentProvider and only escalate to provider-level
failure if it returns falsy). Ensure changes touch the branches around
shouldRetryAlternateKey, rememberFailedKey,
tryResolveAlternateKeyForCurrentProvider and the later failedMap/providerScores
enrichment so provider uptime isn’t penalized when an alternate key recovers the
request.
---
Nitpick comments:
In `@apps/gateway/src/lib/api-key-health.ts`:
- Around line 47-68: The keyHealthMap can grow unbounded when using model-scoped
keys; add a TTL-based eviction to prune entries with no recent activity: augment
KeyHealth entries with a lastActivity timestamp updated in
recordError/recordSuccess, expose getTrackedKeyCount() for monitoring, and
implement a periodic cleanup task (interval based on METRICS_WINDOW_MS or a new
CLEANUP_INTERVAL_MS) that removes map entries whose lastActivity is older than a
configurable TTL (e.g., METRICS_WINDOW_MS or a new KEY_TTL_MS) while preserving
MAX_HISTORY_SIZE semantics; reference keyHealthMap, KeyHealth,
recordError/recordSuccess, getTrackedKeyCount, MAX_HISTORY_SIZE and
METRICS_WINDOW_MS to locate where to update and where to add the cleanup timer.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: fa8012b2-8a2d-4667-8256-d0c644e96a9d
📒 Files selected for processing (21)
apps/gateway/src/api.spec.tsapps/gateway/src/chat/chat.tsapps/gateway/src/chat/tools/get-finish-reason-from-error.spec.tsapps/gateway/src/chat/tools/get-finish-reason-from-error.tsapps/gateway/src/chat/tools/get-provider-env.spec.tsapps/gateway/src/chat/tools/get-provider-env.tsapps/gateway/src/chat/tools/resolve-provider-context.tsapps/gateway/src/chat/tools/retry-with-fallback.spec.tsapps/gateway/src/chat/tools/retry-with-fallback.tsapps/gateway/src/fallback.spec.tsapps/gateway/src/graceful-shutdown.spec.tsapps/gateway/src/lib/api-key-health.spec.tsapps/gateway/src/lib/api-key-health.tsapps/gateway/src/lib/cached-queries.spec.tsapps/gateway/src/lib/cached-queries.tsapps/gateway/src/lib/provider-auth-errors.tsapps/gateway/src/lib/round-robin-env.spec.tsapps/gateway/src/lib/round-robin-env.tsapps/gateway/src/test-utils/mock-openai-server.tsapps/worker/src/services/stats-calculator.spec.tsapps/worker/src/services/stats-calculator.ts
| providerKey = await findCustomProviderKey( | ||
| project.organizationId, | ||
| customProviderName, | ||
| requestId, | ||
| baseModelName, | ||
| ); | ||
| } else { | ||
| providerKey = await findProviderKey( | ||
| project.organizationId, | ||
| usedProvider, | ||
| requestId, | ||
| baseModelName, | ||
| ); |
There was a problem hiding this comment.
Complete the selectionScope propagation for the earlier key lookups.
These lookups are scoped now, but Line 1437 and Lines 1806-1809 / 2457-2460 still call findCustomProviderKey / findProviderKey without the new third argument. That leaves custom-provider validation and direct-provider region locking on global key health, so another model's failures can still hide a healthy key or lock the wrong region.
Also applies to: 2778-2788
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/gateway/src/chat/chat.ts` around lines 2694 - 2704, Calls to
findCustomProviderKey and findProviderKey are missing the new selectionScope
argument in several places; update every invocation (including the earlier call
near the top and the ones that correspond to the referenced blocks) to pass the
current selectionScope value so the key lookups are region/selection-scoped.
Specifically, add selectionScope as the third parameter when calling
findCustomProviderKey(project.organizationId, customProviderName,
selectionScope) and when calling findProviderKey(project.organizationId,
usedProvider, selectionScope) (and similarly for the other direct-provider calls
noted), ensuring you use the same selectionScope variable used elsewhere in this
file so custom-provider validation and region locking are correctly scoped.
| const INVALID_PROVIDER_CREDENTIAL_PATTERNS = [ | ||
| /api key not valid/i, | ||
| /api key not found/i, | ||
| /please pass a valid api key/i, | ||
| ]; |
There was a problem hiding this comment.
Add invalid_api_key code matching to avoid missed credential failures.
At Line 2-Line 4, matching only message text is brittle. Payloads that carry code: "invalid_api_key" with different wording can bypass auth-failure handling, preventing intended retry/blacklist behavior.
🔧 Proposed hardening
const INVALID_PROVIDER_CREDENTIAL_PATTERNS = [
+ /\binvalid_api_key\b/i,
/api key not valid/i,
/api key not found/i,
/please pass a valid api key/i,
+ /incorrect api key provided/i,
];Also applies to: 12-14
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/gateway/src/lib/provider-auth-errors.ts` around lines 1 - 5, The current
INVALID_PROVIDER_CREDENTIAL_PATTERNS only matches human-readable message text
and misses structured error codes like "invalid_api_key"; update the pattern
array (INVALID_PROVIDER_CREDENTIAL_PATTERNS) to include a case-insensitive regex
that matches the error code token (e.g. /\binvalid_api_key\b/i or a JSON-ish
code match) so payloads carrying code: "invalid_api_key" trigger the same
auth-failure handling, and apply the same change to the other identical pattern
array found later in the file (the second occurrence at lines 12-14).
| // Check if this request should fail on the first attempt but succeed on retry | ||
| if (userMessage.includes("TRIGGER_FAIL_ONCE_INVALID_KEY")) { | ||
| failOnceCounter++; | ||
| if (failOnceCounter === 1) { | ||
| c.status(400); | ||
| return c.json({ | ||
| error: { | ||
| message: "API key not valid. Please pass a valid API key.", | ||
| type: "authentication_error", | ||
| param: null, | ||
| code: "invalid_api_key", | ||
| }, | ||
| }); | ||
| } | ||
| // Subsequent requests succeed - fall through to normal response | ||
| } |
There was a problem hiding this comment.
Avoid double-counting failOnceCounter for the invalid-key trigger.
At Line 771, this trigger is a substring match of TRIGGER_FAIL_ONCE. On retries, both handlers run, incrementing the shared counter twice and increasing cross-test coupling.
💡 Suggested fix
-if (userMessage.includes("TRIGGER_FAIL_ONCE_INVALID_KEY")) {
+if (userMessage.includes("TRIGGER_FAIL_ONCE_INVALID_KEY")) {
failOnceCounter++;
if (failOnceCounter === 1) {
c.status(400);
return c.json({
error: {
message: "API key not valid. Please pass a valid API key.",
type: "authentication_error",
param: null,
code: "invalid_api_key",
},
});
}
// Subsequent requests succeed - fall through to normal response
-}
+} else if (userMessage.includes("TRIGGER_FAIL_ONCE")) {
+ // existing generic fail-once logic
+}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/gateway/src/test-utils/mock-openai-server.ts` around lines 770 - 785,
The invalid-key trigger handler (checking userMessage for
"TRIGGER_FAIL_ONCE_INVALID_KEY") is colliding with the more general
"TRIGGER_FAIL_ONCE" handler and causing failOnceCounter to be incremented twice;
make the triggers mutually exclusive by changing the control flow so only one
handler runs: either (A) convert the generic "TRIGGER_FAIL_ONCE" handler to an
else if so it won’t run when "TRIGGER_FAIL_ONCE_INVALID_KEY" matched, or (B)
make the invalid-key check more specific (e.g., exact match or
word-boundary/unique token) so
userMessage.includes("TRIGGER_FAIL_ONCE_INVALID_KEY") cannot also match the
generic "TRIGGER_FAIL_ONCE"; adjust code around failOnceCounter, userMessage,
TRIGGER_FAIL_ONCE_INVALID_KEY and TRIGGER_FAIL_ONCE accordingly.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/gateway/src/chat/chat.ts (1)
4848-4862:⚠️ Potential issue | 🟠 MajorRecovered same-provider retries still mark the provider as failed.
These branches record a failed
routingAttemptbefore rotating to another key on the same provider. Later, Line 5337 and Line 8504 deriveproviderScores.failedfrom any failed attempt keyed only by provider, so a request that recovers on the second key still leaves that provider flagged failed. That keeps the derived provider health/routing stats degraded even though the provider succeeded end-to-end.Please exclude recovered same-provider retries from provider-level failure enrichment, or mark them separately so only unrecovered provider failures affect
providerScores.Also applies to: 4990-5007, 5125-5139, 5252-5269, 8233-8243, 8377-8394
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/gateway/src/chat/chat.ts` around lines 4848 - 4862, The branches that call rememberFailedKey(usedProvider, ...) before rotating to another same-provider key are marking the provider as failed even when tryResolveAlternateKeyForCurrentProvider(true) recovers the request; change the logic so recovered same-provider retries do not count as provider-level failures: either pass a new flag (e.g., excludeProvider=true or keyOnly=true) into rememberFailedKey when same-provider recovery succeeds, or add a routingAttempt property (e.g., recoveredSameProvider) when recording attempts and update the providerScores.failed derivation to ignore attempts with recoveredSameProvider=true; apply the same change to the other listed occurrences (around lines referenced: 4990-5007, 5125-5139, 5252-5269, 8233-8243, 8377-8394) to ensure only unrecovered provider failures affect provider-level health.
♻️ Duplicate comments (1)
apps/gateway/src/chat/chat.ts (1)
1437-1440:⚠️ Potential issue | 🟠 MajorScope the early custom-provider validation lookup too.
Line 1437 still calls
findCustomProviderKey(...)without the new selection scope, so this preflight can still consult global key health and reject a healthy key for the requested model. That leaves the custom-provider path inconsistent with the scoped lookups used later in actual key resolution.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/gateway/src/chat/chat.ts` around lines 1437 - 1440, The preflight call to findCustomProviderKey (assigning customProviderKey) is unscoped and can consult global keys; change this lookup to use the same scoped selection used later in key resolution (i.e., call the scoped variant or pass the selection/scope parameter such as organizationId and requested model/provider) so the early validation only checks keys valid for the project context and model; update the invocation that sets customProviderKey (and any related preflight checks) to use the scoped lookup API used later in the resolution flow (keep references: findCustomProviderKey, customProviderKey, customProviderName).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@apps/gateway/src/chat/chat.ts`:
- Around line 4848-4862: The branches that call rememberFailedKey(usedProvider,
...) before rotating to another same-provider key are marking the provider as
failed even when tryResolveAlternateKeyForCurrentProvider(true) recovers the
request; change the logic so recovered same-provider retries do not count as
provider-level failures: either pass a new flag (e.g., excludeProvider=true or
keyOnly=true) into rememberFailedKey when same-provider recovery succeeds, or
add a routingAttempt property (e.g., recoveredSameProvider) when recording
attempts and update the providerScores.failed derivation to ignore attempts with
recoveredSameProvider=true; apply the same change to the other listed
occurrences (around lines referenced: 4990-5007, 5125-5139, 5252-5269,
8233-8243, 8377-8394) to ensure only unrecovered provider failures affect
provider-level health.
---
Duplicate comments:
In `@apps/gateway/src/chat/chat.ts`:
- Around line 1437-1440: The preflight call to findCustomProviderKey (assigning
customProviderKey) is unscoped and can consult global keys; change this lookup
to use the same scoped selection used later in key resolution (i.e., call the
scoped variant or pass the selection/scope parameter such as organizationId and
requested model/provider) so the early validation only checks keys valid for the
project context and model; update the invocation that sets customProviderKey
(and any related preflight checks) to use the scoped lookup API used later in
the resolution flow (keep references: findCustomProviderKey, customProviderKey,
customProviderName).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 667a06f7-2da5-42a7-a473-1ea11e7d45da
📒 Files selected for processing (4)
apps/gateway/src/chat/chat.tsapps/gateway/src/fallback.spec.tsapps/worker/src/services/stats-calculator.spec.tsapps/worker/src/services/stats-calculator.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/worker/src/services/stats-calculator.ts
- apps/worker/src/services/stats-calculator.spec.ts
- apps/gateway/src/fallback.spec.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 86a36d2c73
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| baseKey: string, | ||
| selectionScope?: string, | ||
| ): string { | ||
| return selectionScope ? `${baseKey}:${selectionScope}` : baseKey; |
There was a problem hiding this comment.
Using selectionScope directly in the map key makes health buckets unbounded, and these bucket keys are never evicted (only history arrays are pruned). In this commit, chat now passes model IDs as scope; for custom providers, parse-model-input accepts arbitrary model names from requests, so callers can create effectively unlimited distinct scopes and grow keyHealthMap without bound over time. This can steadily increase gateway memory usage under high-cardinality/custom-model traffic.
Useful? React with 👍 / 👎.
What changed
This PR now covers the full retry-health and test-hardening diff against
origin/main:API key not validas invalid-credential failures even when the upstream status is400, so same-provider retry can rotate to the next key and permanently blacklist the bad credential.Why
Routing health had started drifting in three different ways:
400payload instead of401/403, which meant the gateway treated the failure as a generic client error and did not rotate to another key.Impact
Main areas changed
apps/gateway/src/chat/chat.tsapps/gateway/src/chat/tools/get-finish-reason-from-error.tsapps/gateway/src/chat/tools/get-provider-env.tsapps/gateway/src/chat/tools/retry-with-fallback.tsapps/gateway/src/chat/tools/resolve-provider-context.tsapps/gateway/src/lib/api-key-health.tsapps/gateway/src/lib/cached-queries.tsapps/gateway/src/lib/provider-auth-errors.tsapps/gateway/src/lib/round-robin-env.tsapps/worker/src/services/stats-calculator.tsValidation
pnpm vitest run apps/gateway/src/fallback.spec.ts apps/worker/src/services/stats-calculator.spec.ts --no-file-parallelismpnpm formatpnpm lintpnpm build